Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add fileExists mock #306

Closed
wants to merge 2 commits into from
Closed

Add fileExists mock #306

wants to merge 2 commits into from

Conversation

cwiggs
Copy link
Contributor

@cwiggs cwiggs commented Oct 6, 2020

This commit adds the fileExist mock that returns "true". Adding it here so people don't have to mock such a simple step when using the JenkinsPipelineUnit framework.

This commit adds the fileExist mock that returns "true".  Adding it here so people don't have to mock such a simple step when using the JenkinsPipelineUnit framework.
@nre-ableton
Copy link
Contributor

nre-ableton commented Oct 6, 2020

I don't think we should add a mock for this. Returning true in all cases makes a pretty broad assumption about what a pipeline is expecting. For example, consider the following example code:

if (fileExists(outputDir)) {
  sh 'make clean'
}
sh 'make'
sh 'make install'

... and now imagine a corresponding unit test:

@Test
void testStuff() {
  // run pipeline
  assertEquals(2, helper.callStack.findAll { call ->
    call.methodName == 'sh'
  }.size())
}

This is a fairly basic example, but my point is, I don't think we can assume that this mock should always return true (or also always return false, for that matter). I realize one can override this by registering a new mock, but the current behavior of leaving this step unmocked means that it is the user's responsibility to decide how this method should behave.

If we were to add a mock for fileExists to this library, I'd prefer it to be similar to the readFile mock, meaning that there would be a separate helper method to register the output that should be returned for a given filename. If you're ok with that solution, I'd be happy to implement it and we can make a corresponding issue to track it.

@stchar stchar self-requested a review October 16, 2020 11:17
Copy link
Contributor

@stchar stchar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nre-ableton is right additional logic is required to filter what files should exist in which tests.

@nre-ableton
Copy link
Contributor

See #312.

@cwiggs
Copy link
Contributor Author

cwiggs commented Oct 20, 2020

#312 Looks good to me, thanks! Closing PR.

@cwiggs cwiggs closed this Oct 20, 2020
@cwiggs cwiggs deleted the patch-1 branch October 20, 2020 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants